Skip to content

Providing pre-built components #172

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
wants to merge 10 commits into from
Closed

Providing pre-built components #172

wants to merge 10 commits into from

Conversation

@ucodia
Copy link

@ucodia ucodia commented Jun 8, 2016

I have seen by looking in the repository history that the library used to provide pre-built components for the most common Bootstrap navigation component (routing tied control). I understand that at the time React Router did not allow to have a wrapper like LinkContainer, thus requiring parallel maintenance on individual components.

Providing pre-built components seems like a good idea as it perfectly matches with the existing paradigm of React Router Link. I assume that they were previously removed from the library because of the maintenance complexity but maybe I am wrong. Given that we now have a stable and more abstract implementation in the form of LinkContainer, it is now possible to implement those pre-built components in the form of thin wrappers. I added implementation for ButtonLink, NavItemLink, MenuItemLink and ListGroupItemLink. Their implementation is very boilerplate and parallel, thus maintenance on those would be in fact minimal as the abstraction now lies on LinkContainer.

For example here is what ButtonLink implementation looks like:

const propTypes = {
  children: React.PropTypes.node,
};

class ButtonLink extends React.Component {
  render() {
    return (
      <LinkContainer {...this.props}>
        <Button>
          {this.props.children}
        </Button>
      </LinkContainer>
    );
  }
}

Also I must mention that those are still allowing the IndexLink behavior by passing the onlyActiveOnIndex as a property.

@jquense
Copy link
Member

jquense commented Jun 8, 2016

Hi there, thanks for the PR. You are correct about the maintenance issues, and I do think that the current state allows for easer maintenance, but as you point out yourself the code to make these components is so minimal, I don't think it adds a lot to provide them (saves a line or two of code) but it also obscures the underlying implementation which is useful to folks in and of itself. i'd say this may be unneeded complexity but others may disagree :)

@taion
Copy link
Member

taion commented Jun 8, 2016

Thanks for the PR! Yeah – I don't think the pre-built components add too much, and it's just a pain to keep things in sync and handle PRs for adding/removing new components. Maybe some HoC-ification might make sense, but as currently implemented, I don't think we want to add these components.

@ucodia
Copy link
Author

ucodia commented Jun 8, 2016

Thanks for your input. The only real vouch I had for adding the components came from the fact that it seems really likely that most users of the library will implement those components in this simplest form. In fact that is why they need the library and therefore the very first thing they might look for in the documentation are things like ButtonLink or NavItemLink.

But then your arguments are reasonable. Between having all user implement the same exact wrapper or provide them with a simple implementation well enough for their needs I was wondering what was the most reasonable choice. So I will keep it separate and close the PR.

@ucodia ucodia closed this Jun 8, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@ucodia @jquense @taion